Skip to content

feat(http): keep slashes in queryParams in redirects #4928

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 1, 2022

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Mar 1, 2022

When a user opens code-server at a url like localhost:8080 without the -e (ignore last opened flag), we redirect to that workspace and append the folder as a query param.

Before, it would encode the url to something like: localhost:8080/?folder=%2FUsers%2Fjp%2Fdev%2Fcoder

Now, it maintains the slashes and redirects to: localhost:8080/?folder=/Users/p/Fdev/coder

Fixes this issue coder/vscode#45 (comment) identified in the VS Code 1.64 update.

This allows us to easily test our redirect path construction logic where we get
the relative path, the query string and construct a redirect path.

By extracting this from `redirect`, we can easily test this logic in a unit
test.

I did this so we could test some logic where slashes in query strings should be
made human-friendly for users.
@jsjoeio jsjoeio added the testing Anything related to testing label Mar 1, 2022
@jsjoeio jsjoeio added this to the March 2022 milestone Mar 1, 2022
@jsjoeio jsjoeio self-assigned this Mar 1, 2022
@jsjoeio jsjoeio temporarily deployed to CI March 1, 2022 16:50 Inactive
@github-actions
Copy link

github-actions bot commented Mar 1, 2022

✨ Coder.com for PR #4928 deployed! It will be updated on every commit.

@jsjoeio jsjoeio force-pushed the jsjoeio/test-redirect branch from 5b78e3c to 31e131b Compare March 1, 2022 17:35
@jsjoeio jsjoeio temporarily deployed to CI March 1, 2022 17:35 Inactive
@jsjoeio jsjoeio changed the title Jsjoeio/test redirect feat(http): keep slashes in queryParams in redirects Mar 1, 2022
@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #4928 (8bc7645) into main (1465d8d) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4928      +/-   ##
==========================================
+ Coverage   69.96%   70.02%   +0.05%     
==========================================
  Files          29       29              
  Lines        1655     1658       +3     
  Branches      364      364              
==========================================
+ Hits         1158     1161       +3     
  Misses        423      423              
  Partials       74       74              
Impacted Files Coverage Δ
src/node/http.ts 69.38% <100.00%> (+0.96%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1465d8d...8bc7645. Read the comment docs.

@jsjoeio jsjoeio temporarily deployed to npm March 1, 2022 17:44 Inactive
@jsjoeio jsjoeio marked this pull request as ready for review March 1, 2022 17:45
@jsjoeio jsjoeio requested a review from a team March 1, 2022 17:45
@jsjoeio jsjoeio mentioned this pull request Mar 1, 2022
28 tasks
@code-asher code-asher temporarily deployed to CI March 1, 2022 18:44 Inactive
@code-asher code-asher temporarily deployed to npm March 1, 2022 18:55 Inactive
@jsjoeio jsjoeio merged commit 506d3f4 into main Mar 1, 2022
@jsjoeio jsjoeio deleted the jsjoeio/test-redirect branch March 1, 2022 19:11
@jsjoeio jsjoeio modified the milestones: March 2022, 4.0.3 Mar 1, 2022
TinLe pushed a commit to TinLe/code-server that referenced this pull request Apr 23, 2022
* refactor(http): extract logic into constructRedirectPath

This allows us to easily test our redirect path construction logic where we get
the relative path, the query string and construct a redirect path.

By extracting this from `redirect`, we can easily test this logic in a unit
test.

I did this so we could test some logic where slashes in query strings should be
made human-friendly for users.

* feat(testing): add tests for constructRedirectPath

Co-authored-by: Asher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Anything related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants